codegen_llvm/llvm_type: avoid matching on the Rust type #115240
Conversation
|
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
r? @bjorn3 |
|
I just found this which seems equally suspicious: rust/compiler/rustc_codegen_llvm/src/type_of.rs Lines 306 to 308 in 4354192 Moreover, pointee types are removed, so the entire hack is probably unnecessary? |
|
Some changes occurred in compiler/rustc_codegen_gcc cc @antoyo |
|
I've removed that other Cranelift is extremely reliant on Rust types in its backend type computation functions, which is concerning... maybe it's just somehow not relevant for ABI there? |
This comment has been minimized.
This comment has been minimized.
|
cg_clif should not be dependent on rust types for calling convention handling. It should only use rust types for handling locals. |
it was already unused before, but due to the recursion the compiler did not realize
|
Okay, I was probably looking at the wrong code then. |
|
@bors r+ |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#115164 (MIR validation: reject in-place argument/return for packed fields) - rust-lang#115240 (codegen_llvm/llvm_type: avoid matching on the Rust type) - rust-lang#115294 (More precisely detect cycle errors from type_of on opaque) - rust-lang#115310 (Document panic behavior across editions, and improve xrefs) - rust-lang#115311 (Revert "Suggest using `Arc` on `!Send`/`!Sync` types") - rust-lang#115317 (Devacationize oli-obk) - rust-lang#115319 (don't use SnapshotVec in Graph implementation, as it looks unused; use Vec instead) - rust-lang#115322 (Tweak output of `to_pretty_impl_header` involving only anon lifetimes) r? `@ghost` `@rustbot` modify labels: rollup
| // This must produce the same result for `repr(transparent)` wrappers as for the inner type! | ||
| // In other words, this should generally not look at the type at all, but only at the | ||
| // layout. |
There was a problem hiding this comment.
Sorry for seeing this so late, but, if not made clear so far by anyone else:
the special cases you removed are simultaneously sound and obsolete.
They're sound because they only affect pointer types' pointee types (which doesn't impact ABI on it own), and they're obsolete thanks to LLVM opaque pointers (making the removal welcome!).
You can kinda tell they were working fine because e.g. Option<&String> would look like i64*, whereas the inner &String would actually be {i8*, i64, i64}* - worst case, it would pessimize some badly written LLVM optimizations, but all of those concerns should be gone now.
In fact, some of this code can very likely be simplified!
scalar_pair_element_llvm_type only exists in the form it does because it needed to special-case wide pointers, but now each component of the scalar pair can be independently converted to LLVM.
It might even make sense to remove caches like scalar_lltypes and instead have something much simpler keyed by Primitive - the existing scalar_llvm_type_at might be good enough, even, and I would argue the cache only existed because of the pointer cases, and can now be fully removed.
There was a problem hiding this comment.
the special cases you removed are simultaneously sound and obsolete.
Yes that was my conclusion, too. That still makes this PR a good cleanup, no? :)
But looking at these special cases led me to ones that aren't sound (see the recent ABI issues I opened), so the cleanup of obsolete-but-sound type matching was still worth it.
I'm sure there's further simplification possible.
codegen_llvm/llvm_type: avoid matching on the Rust type This `match` is highly suspicious. Looking at `scalar_llvm_type_at` I think it makes no difference. But if it were to make a difference that would be a huge problem, since it doesn't look through `repr(transparent)`! Cc `@eddyb` `@bjorn3`
This
matchis highly suspicious. Looking atscalar_llvm_type_atI think it makes no difference. But if it were to make a difference that would be a huge problem, since it doesn't look throughrepr(transparent)!Cc @eddyb @bjorn3